Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GLTFLoader: Expose GLTFParser. #24764

Closed
wants to merge 1 commit into from
Closed

GLTFLoader: Expose GLTFParser. #24764

wants to merge 1 commit into from

Conversation

Hoodgail
Copy link
Contributor

@Hoodgail Hoodgail commented Oct 9, 2022

I want to utilize GLTFParser without needing to clone the script in my own project since threejs' typescript exports it where the script does not.

I want to utilize GLTFParser without needing to clone the script in my own project since threejs' typescript exports it where the script does not.
@Mugen87 Mugen87 changed the title export GLTFParser GLTFLoader: Expose GLTFParser. Oct 10, 2022
@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 10, 2022

since threejs' typescript exports it where the script does not.

Well, that means the TS declaration file is wrong. However, I can see that exposing GLTFParser might be useful for certain use case although the user level code has to take care about the extension and plugin configuration.

@Hoodgail
Copy link
Contributor Author

One of the reason i wanted the GLTFParser was because GLTFLLoader["parse"] function only accepts array buffer( which gets converted to a json string) or just a json string.

But i'm using a binary serialization format rather than json strings, and when it gets decoded in JS i have to manually turn the json into a string with JSON.stringify then send it to GLTFLLoader["parse"] which just does JSON.parse again inside of the function, this affects performance. I can't just send the json my self.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Oct 10, 2022

/cc @takahirox, any preference here?

I am not hugely enthusiastic about all of GLTFParser becoming a public API. Its implementation is something we will, more likely than not, make breaking changes to over time. That said ... extension authors already need to use its API, that's unavoidable, so if you know what you're getting into, maybe we should allow it. See also, #24506 (comment).

GLTFLLoader["parse"] function only accepts array buffer ( which gets converted to a json string)

This isn't quite correct. The header chunk of the binary GLB array buffer is JSON, and is decoded to text before calling JSON.parse(). But the larger payload of a GLB is (or should be, in any efficient exporter) binary data, which is never converted to text or JSON when parsing a GLB. This contains the vertex, texture, animation, and other large data. So if you find yourself dealing with large quantities of JSON, you may have some poorly-optimized files.

@takahirox
Copy link
Collaborator

I am not hugely enthusiastic about all of GLTFParser becoming a public API.

Same here. GLTFParser is primarily designed for GLTFLoader internal use. We may break the APIs anytime. If we want to expose GLTFParser we first need to clarify which methods and properties of GLTFParser should be public and keep unchanged.

@Hoodgail
Copy link
Contributor Author

Hoodgail commented Oct 11, 2022

I understand what GLTFParser being public might cause.
But don't things get changed all the time?

But about GLTFLoader.parse why can't i just manually pass the the json, because i'm using the default gltf json rather than the GLB binary data.

The binary serialization format is a custom format, not GLB, when it gets encoded after its loader, it just turns into a normal GLTF json.

But i have to then stringify the json and then send it to GLTFLoader.parse(string) then it does parses it again within the function.

GLTFLoader.parse(json)
image

image

Sorry, i'm not sure how else i could explain this issue

@takahirox
Copy link
Collaborator

takahirox commented Oct 13, 2022

I don't remember why GLTFLoader.parse doesn't take json object... @donmccurdy Do you know the reason? I feel it may be ok to accept json object.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Oct 14, 2022

I'd be fine with the method also accepting the object, already parsed from JSON. Much simpler than exposing the parser.

This does mean your binary data is in base64 Data URIs, though. That's less efficient to parse than a GLB would be, so just be aware that loading will be considerably slower as a result.

@Hoodgail
Copy link
Contributor Author

No, my binary data is not in base64, i'm doing something else for that too. But yes, being able to accept jsons is much better, i feel like this should be done with all of the others loaders too, should i commit a pull request?

@takahirox
Copy link
Collaborator

should i commit a pull request?

Yes, we are happy if you make and send a PR, thanks!

Hoodgail added a commit to Hoodgail/three.js that referenced this pull request Oct 15, 2022
This pull requests will additionally allow you to pass through parsed json objects rather than only Buffers and strings.

mrdoob#24764
@Hoodgail
Copy link
Contributor Author

Hoodgail commented Oct 15, 2022

should i commit a pull request?

Yes, we are happy if you make and send a PR, thanks!

@takahirox done #24801

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 17, 2022

Closing in favor of #24801.

@Mugen87 Mugen87 closed this Oct 17, 2022
Mugen87 pushed a commit that referenced this pull request Oct 17, 2022
* GLTFLoader: Allow json objects

This pull requests will additionally allow you to pass through parsed json objects rather than only Buffers and strings.

#24764

* Spaces after and before brackets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants